Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move assert_matches to an inner module #86947

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Jul 7, 2021

Fixes #82913

@m-ou-se m-ou-se added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 7, 2021
@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 7, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 changed the title Move assert_matches to a submodule. Move assert_matches to an inner module Jul 7, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 7, 2021

(I edited the name because "submodule" has a specific meaning WRT git, and I was confused at first.)

@m-ou-se m-ou-se force-pushed the assert-matches-to-submodule branch from b9d41e6 to 9c1573a Compare July 7, 2021 23:23
@rust-log-analyzer

This comment has been minimized.

@m-ou-se m-ou-se force-pushed the assert-matches-to-submodule branch 2 times, most recently from 052ea30 to 23bc386 Compare July 8, 2021 00:05
@rust-log-analyzer

This comment has been minimized.

@m-ou-se m-ou-se force-pushed the assert-matches-to-submodule branch from 23bc386 to e304443 Compare July 8, 2021 00:33
@m-ou-se m-ou-se added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 8, 2021
@joshtriplett joshtriplett added beta-accepted Accepted for backporting to the compiler in the beta channel. beta-nominated Nominated for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jul 14, 2021
@yaahc
Copy link
Member

yaahc commented Jul 14, 2021

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jul 14, 2021

Team member @yaahc has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 14, 2021
@rfcbot
Copy link

rfcbot commented Jul 14, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 14, 2021
@yaahc
Copy link
Member

yaahc commented Jul 15, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jul 15, 2021

📌 Commit e304443 has been approved by yaahc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 15, 2021
…, r=yaahc

Move assert_matches to an inner module

Fixes rust-lang#82913
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 15, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#85579 (Added Arc::try_pin)
 - rust-lang#86478 (Add -Zfuture-incompat-test to assist with testing future-incompat reports.)
 - rust-lang#86947 (Move assert_matches to an inner module)
 - rust-lang#87081 (Add tracking issue number to `wasi_ext`)
 - rust-lang#87127 (Add safety comments in private core::slice::rotate::ptr_rotate function)
 - rust-lang#87134 (Make SelfInTyParamDefault wording not be specific to type defaults)
 - rust-lang#87147 (Update cargo)
 - rust-lang#87154 (Fix misuse of rev attribute on <a> tag)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a5acb7b into rust-lang:master Jul 15, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 15, 2021
@richkadel
Copy link
Contributor

@m-ou-se @yaahc

This change is causing lots of downstream build failures (compiling Fuchsia for instance).

Choosing to call this std::assert::assert_matches! is causing compiler errors in code that explicitly references (e.g.,use) the macro std::assert!, because they are at the same item path as the parent mod: std::assert or core::assert now refer to both the default assert! macro and the mod containing assert_matches.

In fact, the feature gate for assert_matches generates compile time errors for legitimate references to the std::assert macro (Rust tries to apply the feature gate when it incorrectly assumes the reference is to the parent mod of assert_matches! making the error message unintuitive.

Example, in some code I'm trying to compile since this landed, the original author needs to specify an absolute package path to, for instance, deconflict with a different item in the local mod, with the name assert.

I think it might be better to use an item path that isn't already in use, like one of:

  • std::assert_matches::assert_matches would at least match the feature gate name (I mention this because it was confusing at first that the error messages I got recommended enabling the feature(assert_matches) just to use core::assert.
  • std::asserts::assert_matches is a shorter alternative you might also consider

cc: @tmandry @anp

@m-ou-se
Copy link
Member Author

m-ou-se commented Jul 16, 2021

Oh, good catch. Let's move it to std::macros or something for now. The name doesn't matter much, since the module is unstable.

@yaahc , do you have time to do this and handle the backport too?

@richkadel
Copy link
Contributor

I added #87189 for tracking purposes

@RalfJung
Copy link
Member

@rust-lang/infra this PR changes rustc_mir/interpret but the bot failed to do the usual ping... is this a known problem? (The change is fine of course.)

I assume the change was not present in the PR at the beginning and got added by pushing to the branch later.

@jyn514
Copy link
Member

jyn514 commented Jul 16, 2021

@RalfJung yes: rust-lang/highfive#223

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2021
…i-obk

rename assert_matches module

Fixes nightly breakage introduced in rust-lang#86947
@m-ou-se m-ou-se deleted the assert-matches-to-submodule branch July 20, 2021 20:05
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 22, 2021
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.55.0, 1.54.0 Jul 22, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 22, 2021
…ulacrum

[beta] backports

Backports:

* Move assert_matches to an inner module rust-lang#86947
* rename assert_matches module rust-lang#87195
* Fix rust-analyzer install when not available. rust-lang#87007
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 24, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there a gentler way to land the assert_matches macro?